DataNodeExtractTrans implementation by LonelyCat124 · Pull Request #3301 · stfc/PSyclone

LonelyCat124 · 2026-01-26T15:19:16Z

Initial implementation - transformation is quite straightforward so should be good for a review assuming CI is ok.

codecov · 2026-01-26T15:37:10Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.95%. Comparing base (59f796e) to head (5c5fea5).
⚠️ Report is 18 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3301   +/-   ##
=======================================
  Coverage   99.95%   99.95%           
=======================================
  Files         380      381    +1     
  Lines       53949    54020   +71     
=======================================
+ Hits        53927    53998   +71     
  Misses         22       22

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

LonelyCat124 · 2026-01-26T15:41:53Z

@sergisiso @arporter This is ready for a first review. The implementation is currently pretty small and neat - if you want more functionality testing let me know and I can add more.

arporter

Thanks very much Aidan - as you say it's pleasingly simple. Also as you say, it could do with some more checks and associated tests :-)

On the naming front, I worry that the use of Extract will be confused with the existing ExtractTrans which is obviously quite different. IAAI (two of them) and it seems that IntroduceLocal/TemporaryTrans is a good alternative that doesn't involve "Extract".

Finally, (once everything else is done) I think it would be exciting (and possibly educational) to try this for real. I'm thinking we'd alter nemo/scripts/utils.py::normalise_loops so that it e.g. looks for any Calls which have array expressions as arguments.

src/psyclone/psyir/transformations/__init__.py

src/psyclone/psyir/transformations/datanode_extract_trans.py

arporter · 2026-01-27T10:15:45Z

src/psyclone/psyir/transformations/datanode_extract_trans.py

+        """Applies the DataNodeExtractTransApplies to the input arguments.
+
+        :param node: The datanode to extract.
+        :param storage_name: The name of the temporary variable to store


Perhaps "The (base) name of the..." to indicate that it won't necessarily be exactly that name that is used. Also, please could you de-dent the following lines to just use four spaces so we don't need as many lines.

I guess we need to decide if this should be a base name of if the storage_name argument is provided the name must be exact? Happy to go either way - if the name must be exact I should probably add it to validate.

I can't imagine anyone ever caring that it was exact so I vote for it just being a suggestion (i.e. base_name).

Changed this behaviour now.

src/psyclone/psyir/transformations/datanode_extract_trans.py

src/psyclone/tests/psyir/transformations/datanode_extract_trans_test.py

src/psyclone/psyir/transformations/datanode_extract_trans.py

…e into 1646_datanode_extract_trans

LonelyCat124 · 2026-02-03T13:50:12Z

@arporter I think I fixed all the issues I found from the last review, ready for another look.

arporter

Looking good @LonelyCat124 - I like your solution for the imported symbols.
I think it would be worth doing the renaming of the transformation now. Please could you also add it to the User Guide.
I've addded a real-life NEMO example to #1958 - please could you use that as the basis for an additional test?

arporter · 2026-02-05T13:13:22Z

src/psyclone/psyir/transformations/datanode_extract_trans.py

+                    scoped_name_sym = scope_symbols.get(sym.name, None)
+                    if scoped_name_sym and sym is not scoped_name_sym:
+                        raise TransformationError(
+                            f"Input node contains an imported symbol whose "


It took me a while to work out what this check was for. Could you extend the text to say that the "The type of the node supplied to {self.name} depends upon an imported symbol ('{sym.name}') which has a name clash with a symbol in the current scope."

arporter · 2026-02-05T13:15:12Z

src/psyclone/psyir/transformations/datanode_extract_trans.py

+                            f"DataNodeExtractTrans cannot be applied. "
+                            f"Clashing symbol name is '{sym.name}'."
+                        )
+                    # If its an imported symbol we need to check if its


I think we want this check as part of the previous one as we may have the same symbol imported from the same location but represented by different Symbol instances. i.e. sym is not scoped_name_sym but, if they are both imported from the same module, they must be representing the same variable.

@arporter I did wonder about this initially, but couldn't find a way to create a test where it happened, so abandoned it, but I'll have another go since you also think it should be possible.

Yeah I found a way to test this now when looking at another comment.

Done I think - at least for validate, need to think about apply.

Actually apply should be ok, but i'm a bit confused, i make symbol copies and put them in the symbol table and then just don't reference them anywhere (including not reforming the shape appropriately) so I'm a little confused.

I'm so confused by symbols every time i try to work with them, I think I need to pause and double check all the symbols being created are used where i expect afterwards and are in the symbol table.

Fixed now I think.

arporter · 2026-02-05T13:17:32Z

src/psyclone/psyir/transformations/datanode_extract_trans.py

                        f"so the DataNodeExtractTrans cannot be applied. "
                        f"Input node was '{node.debug_string().strip()}'."
                    )
+                # Otherwise we have an ArrayBounds


I need a bit more help following this code. Perhaps, "...ArrayBounds - check to see which Symbols are referenced in those bounds."

arporter · 2026-02-05T13:18:54Z

src/psyclone/psyir/transformations/datanode_extract_trans.py

+                    symbols.update(element.lower.get_all_accessed_symbols())
+                if isinstance(element.upper, DataNode):
+                    symbols.update(element.upper.get_all_accessed_symbols())
+                scope_symbols = node.scope.symbol_table.get_symbols()


"Compare these symbols with those already in scope at node"

arporter · 2026-02-05T13:24:01Z

src/psyclone/psyir/transformations/datanode_extract_trans.py

+                                f"whose containing module collides with an "
+                                f"existing symbol. Colliding name is "
+                                f"'{sym.interface.container_symbol.name}'."
+                                )


Unfortunately we also have to allow for a situation when the type of the imported variable (a_var in this case) depends on private variables:

module some_other_mod integer, parameter :: dim1 = 4, dim2 = 5 real(kind=wp), dimension(dim1, dim2) :: a_var public :: a_var private end module

in which case we can't import the symbols that define the shape of the array. You'll need to check their visibility in the symbol_table associated with the Container pointed to by the import interface of a_var.

@arporter I'm a bit confused when trying to do this test - when I examine the interface of dim1 and dim2 (as found in the size of the a_var.datatype)I don't get an ImportInterface but instead a StaticInterface. Is this expected behaviour? If so, how can I differentiate between two private static variables where one is in the current scope and one is from a "resolve imports" module?

Yeah, I'm not sure how to tell the difference between the dim/dim2 symbols in your example, and x in this code:

implicit none integer, parameter, private :: x = 1 private contains subroutine sub() integer :: i i = x end subroutine sub end module test

I could check that if its a static variable then it has to be in the scope? But why are those not ImportInterface symbols despite being static?

arporter · 2026-02-05T13:49:31Z

src/psyclone/tests/psyir/transformations/datanode_extract_trans_test.py

+    code = """subroutine test()
+        use a_mod, only: some_var
+        use some_mod, only: j
+        j = some_var(1,3)


Picky but j is a parameter so can't be assigned to.

arporter · 2026-02-05T13:50:30Z

src/psyclone/tests/psyir/transformations/datanode_extract_trans_test.py

+    assign = psyir.walk(Assignment)[0]
+    with pytest.raises(TransformationError) as err:
+        dtrans.validate(assign.rhs)
+    assert ("Input node contains an imported symbol whose containing module "


"...symbol ('sym.name') whose ..."

arporter · 2026-02-05T13:53:05Z

src/psyclone/tests/psyir/transformations/datanode_extract_trans_test.py

+    psyir = fortran_reader.psyir_from_source(code)
+    assign = psyir.walk(Assignment)[0]
+    dtrans.apply(assign.rhs, storage_name="temporary")
+    out = fortran_writer(psyir)


Pls could you add checks that the code compiles. (assert Compile(tmpdir).string_compiles(out))
Also, following @hiker pointing out that tmp_path is the replacement for tmpdir, please could you switch to using that?

arporter · 2026-02-05T13:55:47Z

src/psyclone/tests/psyir/transformations/datanode_extract_trans_test.py

+    assert """  tmp = 3 * b
+  a(:4) = tmp""" in out
+
+    # Test the imports are handled correctly.


I think the following could be a separate test.

arporter · 2026-02-05T13:58:05Z

src/psyclone/tests/psyir/transformations/datanode_extract_trans_test.py

+    assign = psyir.walk(Assignment)[0]
+    dtrans.apply(assign.rhs)
+    out = fortran_writer(psyir)
+    assert """  integer, dimension(25,50) :: tmp


Compilation checks for these various checks please (if possible).

LonelyCat124 added 2 commits January 26, 2026 15:07

DataNodeExtractTrans implementation

1adc9ce

Merge branch 'master' into 1646_datanode_extract_trans

d4d7510

LonelyCat124 requested review from arporter and sergisiso January 26, 2026 15:41

LonelyCat124 added the ready for review label Jan 26, 2026

arporter added under review and removed ready for review labels Jan 27, 2026

arporter requested changes Jan 27, 2026

View reviewed changes

arporter added reviewed with actions and removed under review labels Jan 27, 2026

LonelyCat124 added 9 commits January 27, 2026 14:51

Fixes for review

8c99aa3

Added test for import resolution

dd30fe9

Some more changes towards review

f81a2c3

Merge branch 'master' into 1646_datanode_extract_trans

9a28c7b

Merge branch '1646_datanode_extract_trans' of github.com:stfc/PSyclon…

13db8c7

…e into 1646_datanode_extract_trans

Interface handling

731bb59

Fixed incorrect behaviour due to wrong variable usage

ffa39bc

Missing coverage fixes

b3a90fc

fixed error in validate and coverage

5c5fea5

LonelyCat124 requested a review from arporter February 3, 2026 13:41

LonelyCat124 added ready for review and removed reviewed with actions labels Feb 3, 2026

arporter added under review and removed ready for review labels Feb 5, 2026

arporter mentioned this pull request Feb 5, 2026

Transformation to move (array?) operations outside call arguments #1958

Open

arporter requested changes Feb 5, 2026

View reviewed changes

arporter removed the under review label Feb 5, 2026

arporter added the reviewed with actions label Feb 5, 2026

Conversation

LonelyCat124 commented Jan 26, 2026

Uh oh!

codecov bot commented Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

LonelyCat124 commented Jan 26, 2026

Uh oh!

arporter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LonelyCat124 commented Feb 3, 2026

Uh oh!

arporter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

codecov bot commented Jan 26, 2026 •

edited

Loading